detox/e2e/support/utils/index.ts (1)

308-331: Redundant require('detox') calls in final checks.

Both waitForElementToNotExist and waitForElementToExist import detoxExpect at the start of the function, then import it again as detoxExpectFinal for the final check outside the loop. Since the same detoxExpect reference would work, the second import is unnecessary.

♻️ Proposed simplification

 export async function waitForElementToNotExist(
     detoxElement: Detox.NativeElement,
     timeout: number = timeouts.HALF_MIN,
     pollInterval: number = timeouts.HALF_SEC,
 ): Promise<void> {
     const {expect: detoxExpect} = require('detox');
     const startTime = Date.now();
     /* eslint-disable no-await-in-loop */
     while (Date.now() - startTime < timeout) {
         try {
             await detoxExpect(detoxElement).not.toExist();
             return; // Element no longer exists
         } catch (error) {
             if ((Date.now() - startTime) + pollInterval >= timeout) {
                 throw error;
             }
             await wait(pollInterval);
         }
     }
     /* eslint-enable no-await-in-loop */
     // Final check - will throw if still exists
-    const {expect: detoxExpectFinal} = require('detox');
-    await detoxExpectFinal(detoxElement).not.toExist();
+    await detoxExpect(detoxElement).not.toExist();
 }

Apply the same pattern to waitForElementToExist.

Also applies to: 353-376

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/support/utils/index.ts` around lines 308 - 331, The final explicit
require('detox') call is redundant: in waitForElementToNotExist (and similarly
in waitForElementToExist) you already destructured detoxExpect at the top;
remove the second require and reuse the initial detoxExpect instead of
detoxExpectFinal for the final check and any other post-loop assertions to avoid
duplicate requires and keep behavior identical.

detox/e2e/test/products/channels/ipad/ipad_post_message.e2e.ts (2)

136-140: Redundant assertion after waitFor.

At line 139, await expect(postListPostItem).toExist() immediately follows await waitFor(postListPostItem).toExist().withTimeout(...) at line 138. If the waitFor succeeds, the expect is guaranteed to pass and adds no value.

♻️ Remove redundant assertion

         const {post} = await Post.apiGetLastPostInChannel(siteOneUrl, testChannel.id);
         const {postListPostItem} = ChannelScreen.getPostListPostItem(post.id, message);
         await waitFor(postListPostItem).toExist().withTimeout(timeouts.TEN_SEC);
-        await expect(postListPostItem).toExist();
     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/test/products/channels/ipad/ipad_post_message.e2e.ts` around lines
136 - 140, The second explicit assertion is redundant: after awaiting
waitFor(postListPostItem).toExist().withTimeout(...), the element existence is
already verified, so remove the duplicate await
expect(postListPostItem).toExist(); Delete the expect line that references
postListPostItem in the test using ChannelScreen.getPostListPostItem and rely on
the waitFor(...) call to assert existence.

---

61-82: Test doesn't navigate back to channel list at end.

Per coding guidelines, each test should navigate back to channel list at the end for clean state for next test. While beforeEach relaunches the app on iPad which provides isolation, for consistency and to align with guidelines, consider adding navigation back to channel list.

This applies to all five tests in this file (MM-TIPAD_15 through MM-TIPAD_19).

♻️ Proposed fix for MM-TIPAD_15

         // * Verify the sidebar is still visible after posting (tablet split view).
         // On iPad the software keyboard (UITextEffectsWindow) may cover the lower portion of
         // the channel_list.screen SafeAreaView, dropping its visible percentage below the 75%
         // threshold required by toBeVisible().  Use toExist() to confirm the view is in the
         // hierarchy regardless of transient keyboard obstruction.
         await waitFor(ChannelListScreen.channelListScreen).toExist().withTimeout(timeouts.TEN_SEC);
+
+        // # Navigate back to channel list for clean state
+        await ChannelScreen.back();
     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/test/products/channels/ipad/ipad_post_message.e2e.ts` around lines
61 - 82, The test MM-TIPAD_15 does not navigate back to the channel list at the
end; update the test to return the app to the channel list after the final
assertion by invoking the navigation helper (e.g., call
ChannelScreen.backToChannelList() — or the existing helper that performs the
same action) immediately after the
waitFor(ChannelListScreen.channelListScreen)...withTimeout(...) line so the test
leaves the app in the channel-list state for subsequent tests; apply the same
change to MM-TIPAD_16 through MM-TIPAD_19.

detox/e2e/test/products/channels/channels/archive_channel.e2e.ts (2)

105-130: Android fallback in closeBrowseChannelsChannel may cause unexpected navigation.

In the Android branch (lines 112-121), if the close button isn't found, the code falls back to device.pressBack(). However, if Browse Channels was already dismissed, pressBack() might navigate further back than intended (e.g., to a previous screen in the stack).

Consider whether the catch block should simply do nothing (like the iOS branch) rather than calling pressBack().

♻️ Proposed fix for safer fallback

     if (isAndroid()) {
         // On Android the modal stack can collapse differently — use pressBack
         // which reliably dismisses the current screen regardless of stack depth.
         try {
             await waitFor(BrowseChannelsScreen.closeButton).toExist().withTimeout(timeouts.FOUR_SEC);
             await BrowseChannelsScreen.closeButton.tap();
         } catch {
             // Browse Channels was already dismissed when Channel.back() popped the stack
-            await device.pressBack();
+            // No action needed — navigation already complete
         }
     } else {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/test/products/channels/channels/archive_channel.e2e.ts` around
lines 105 - 130, The Android fallback in closeBrowseChannelsChannel currently
calls device.pressBack() inside the catch when BrowseChannelsScreen.closeButton
isn't found, which can over-navigate; change the catch to mirror the iOS branch
by doing nothing (no pressBack) or alternatively perform a safe existence check
before calling device.pressBack(); update the catch block that references
BrowseChannelsScreen.closeButton and device.pressBack so that if the close
button is missing after ChannelScreen.back()/switchToChannelById, the function
simply returns without invoking device.pressBack().

---

666-733: Search test properly handles synchronization around tapReturnKey.

The test at MM-T1679_1 correctly disables synchronization before tapReturnKey() and re-enables it after waiting for results. However, if waitForElementToBeVisible throws, synchronization would be left disabled.

♻️ Proposed fix for synchronization safety

         // * Verify the search result contains the message from the archived channel
         // Disable synchronization before tapReturnKey: the search keeps the dispatch
         // queue busy while processing network/DB work, which would otherwise cause
         // tapReturnKey to block indefinitely waiting for Detox idle.
         await device.disableSynchronization();
-        await SearchMessagesScreen.searchInput.tapReturnKey();
-
-        // Wait for the search result text to appear (text element, not the composed matcher
-        // which uses withDescendant and can be unreliable when text is highlighted/split).
-        const searchResultText = element(
-            by.
-                text(uniqueMessage).
-                withAncestor(by.id(SearchMessagesScreen.postList.testID.flatList)),
-        );
-        await waitForElementToBeVisible(searchResultText, timeouts.ONE_MIN);
-        await device.enableSynchronization();
+        try {
+            await SearchMessagesScreen.searchInput.tapReturnKey();
+
+            // Wait for the search result text to appear (text element, not the composed matcher
+            // which uses withDescendant and can be unreliable when text is highlighted/split).
+            const searchResultText = element(
+                by.
+                    text(uniqueMessage).
+                    withAncestor(by.id(SearchMessagesScreen.postList.testID.flatList)),
+            );
+            await waitForElementToBeVisible(searchResultText, timeouts.ONE_MIN);
+        } finally {
+            await device.enableSynchronization();
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/test/products/channels/channels/archive_channel.e2e.ts` around
lines 666 - 733, The test disables Detox synchronization before calling
SearchMessagesScreen.searchInput.tapReturnKey() but re-enables it only after
waitForElementToBeVisible, so if that wait throws the test leaves
synchronization disabled; wrap the disable/enable sequence in a try/finally
(call device.disableSynchronization() then try { tapReturnKey(); await
waitForElementToBeVisible(searchResultText, ...) } finally {
device.enableSynchronization() }) to guarantee device.enableSynchronization()
always runs; update the MM-T1679_1 test around
SearchMessagesScreen.searchInput.tapReturnKey / waitForElementToBeVisible to use
this pattern.

detox/e2e/support/ui/screen/thread.ts (1)

91-107: Tooltip dismissal could verify dismissal completed.

The dismissScheduledPostTooltip method taps the close button but doesn't verify the tooltip actually disappeared. The relevant code snippet from account.ts (lines 115-125) shows a more robust pattern that waits for the tooltip to not exist after tapping.

This is a minor concern since the wait(HALF_SEC) provides some buffer, but for reliability on slower CI environments, consider adding the verification.

♻️ Proposed improvement for robustness

     dismissScheduledPostTooltip = async () => {
         // Try to close scheduled post tooltip if it exists (try both regular and admin account versions)
         try {
             await waitFor(this.scheduledPostTooltipCloseButton).toBeVisible().withTimeout(timeouts.FOUR_SEC);
             await this.scheduledPostTooltipCloseButton.tap();
-            await wait(timeouts.HALF_SEC);
+            await waitFor(this.scheduledPostTooltipCloseButton).not.toExist().withTimeout(timeouts.FIVE_SEC);
         } catch {
             // Try admin account version
             try {
                 await waitFor(this.scheduledPostTooltipCloseButtonAdminAccount).toBeVisible().withTimeout(timeouts.FOUR_SEC);
                 await this.scheduledPostTooltipCloseButtonAdminAccount.tap();
-                await wait(timeouts.HALF_SEC);
+                await waitFor(this.scheduledPostTooltipCloseButtonAdminAccount).not.toExist().withTimeout(timeouts.FIVE_SEC);
             } catch {
                 // Tooltip not visible, continue
             }
         }
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/support/ui/screen/thread.ts` around lines 91 - 107, The
dismissScheduledPostTooltip function currently taps the close buttons
(scheduledPostTooltipCloseButton and
scheduledPostTooltipCloseButtonAdminAccount) but only waits a fixed HALF_SEC;
update both tap paths in dismissScheduledPostTooltip to explicitly verify the
tooltip is gone after tapping by waiting for the corresponding tooltip element
to be not visible/not exist (use the same waitFor(...).toBeNotVisible() or
.toNotExist() pattern used in account.ts) with an appropriate timeout instead of
relying on wait(timeouts.HALF_SEC), so each branch confirms dismissal completed.

detox/e2e/support/ui/screen/channel.ts (1)

420-446: Comment about locale-dependent behavior is incorrect.

The comment at line 442 mentions moment.weekday() being locale-dependent, but the code uses new Date().getDay() which returns 0-6 (Sunday-Saturday) regardless of locale. The comment should be updated to reflect the actual implementation.

Additionally, the logic for Sunday (day 0) falls through to scheduleMessageForMonday() per the comment at lines 441-444, but the docstring at line 424 states Sunday should use "Tomorrow". Consider clarifying which behavior is intended.

📝 Proposed comment fix

     scheduleMessageForAvailableOption = async () => {
         const day = new Date().getDay(); // 0 = Sunday … 6 = Saturday
         if (day === 5 || day === 6) {
             // Friday or Saturday: only "Monday" is available
             await this.scheduleMessageForMonday();
         } else if (day === 1) {
             // Monday: "Tomorrow" and "Next Monday" are available; pick "Next Monday"
             await this.scheduleMessageForNextMonday();
         } else {
-            // Sunday, Tuesday–Thursday: "Tomorrow" is available but may not match on
-            // all CI server locales (moment.weekday() is locale-dependent). Use "Monday"
-            // which is always present as a stable fallback on weekdays.
+            // Sunday, Tuesday–Thursday: "Tomorrow" is available. Use "Monday"
+            // as a stable fallback since it's reliably present on these days.
             await this.scheduleMessageForMonday();
         }
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/support/ui/screen/channel.ts` around lines 420 - 446, The docstring
and inline comment for scheduleMessageForAvailableOption are inconsistent and
incorrect about locale dependence: replace the mention of moment.weekday() with
an accurate note that new Date().getDay() returns 0–6 (Sunday–Saturday)
regardless of locale, and then reconcile the Sunday behavior—either change the
code to call scheduleMessageForTomorrow when day === 0 (if you intend Sunday to
pick "Tomorrow") or update the docstring's schedule table to state that Sunday
falls through to scheduleMessageForMonday; adjust the comment block and any
mention near scheduleMessageForMonday and scheduleMessageForNextMonday
accordingly so comments match the actual logic.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

detox/e2e/test/products/channels/channels/archive_channel.e2e.ts (1)

99-129: Duplicated logic between iOS and Android branches.

The closeBrowseChannelsChannel helper has nearly identical code in both the if (isAndroid()) and else branches. Consider consolidating:

💡 Suggested simplification

 async function closeBrowseChannelsChannel() {
     await ChannelScreen.back();
     await wait(timeouts.ONE_SEC);

-    // After the app awaits switchToChannelById, the Browse Channels modal may already
-    // be dismissed (dismissAllModalsAndPopToScreen closes it during navigation). Check
-    // whether Browse Channels is still on screen before trying to close it.
-    if (isAndroid()) {
-        // On Android the modal stack can collapse differently — tap close button if present.
-        try {
-            await waitFor(BrowseChannelsScreen.closeButton).toExist().withTimeout(timeouts.FOUR_SEC);
-            await BrowseChannelsScreen.closeButton.tap();
-        } catch {
-            // Browse Channels was already dismissed when Channel.back() popped the stack
-            // No action needed — navigation already complete
-        }
-    } else {
-        try {
-            await waitFor(BrowseChannelsScreen.closeButton).toExist().withTimeout(timeouts.FOUR_SEC);
-            await BrowseChannelsScreen.closeButton.tap();
-        } catch {
-            // Browse Channels was already dismissed by switchToChannelById navigation
-        }
-    }
+    // After the app awaits switchToChannelById, the Browse Channels modal may already
+    // be dismissed (dismissAllModalsAndPopToScreen closes it during navigation). Check
+    // whether Browse Channels is still on screen before trying to close it.
+    try {
+        await waitFor(BrowseChannelsScreen.closeButton).toExist().withTimeout(timeouts.FOUR_SEC);
+        await BrowseChannelsScreen.closeButton.tap();
+    } catch {
+        // Browse Channels was already dismissed — navigation already complete
+    }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/test/products/channels/channels/archive_channel.e2e.ts` around
lines 99 - 129, The helper closeBrowseChannelsChannel contains duplicated
try/catch logic in both the if (isAndroid()) and else branches; consolidate into
a single code path that calls ChannelScreen.back(), waits, then runs one try {
await
waitFor(BrowseChannelsScreen.closeButton).toExist().withTimeout(timeouts.FOUR_SEC);
await BrowseChannelsScreen.closeButton.tap(); } catch { /* swallow: Browse
Channels already dismissed */ } so you remove the duplicated branches but keep
the pre-check wait(timeouts.ONE_SEC) and any Android-specific comment if you
want to document modal-stack differences for future readers.

detox/e2e/support/utils/index.ts (1)

308-330: Consider adding diagnostic information to the timeout error.

When waitForElementToNotExist times out, the thrown error comes from the underlying detoxExpect which may not include context about which element was being waited for. This makes debugging flaky tests harder.

💡 Suggested improvement

 export async function waitForElementToNotExist(
     detoxElement: Detox.NativeElement,
     timeout: number = timeouts.HALF_MIN,
     pollInterval: number = timeouts.HALF_SEC,
 ): Promise<void> {
     const {expect: detoxExpect} = require('detox');
     const startTime = Date.now();
     /* eslint-disable no-await-in-loop */
     while (Date.now() - startTime < timeout) {
         try {
             await detoxExpect(detoxElement).not.toExist();
             return; // Element no longer exists
         } catch (error) {
             if ((Date.now() - startTime) + pollInterval >= timeout) {
-                throw error;
+                throw new Error(`Element still exists after ${timeout}ms timeout`);
             }
             await wait(pollInterval);
         }
     }
     /* eslint-enable no-await-in-loop */
     // Final check - will throw if still exists
     await detoxExpect(detoxElement).not.toExist();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/support/utils/index.ts` around lines 308 - 330, The timeout error
from waitForElementToNotExist lacks contextual diagnostics; update the function
(referencing waitForElementToNotExist, detoxElement, detoxExpect, timeout and
pollInterval) to catch the underlying errors both inside the loop and on the
final check and rethrow a new Error that includes identifying context (e.g.,
stringified detoxElement or a brief selector/description and the timeout value)
plus the original error message/stack (attach as cause or include text) so test
logs show which element and timeout caused the failure.

detox/e2e/support/ui/screen/channel.ts (1)

420-446: Day-of-week logic comment is slightly inconsistent with implementation.

The comment states:

• "Sunday (0): Tomorrow"
• "Tue–Thu (2–4): Tomorrow, Monday"

But the implementation (lines 441-444) uses scheduleMessageForMonday() for Sunday and Tuesday–Thursday, not "Tomorrow". While this works (Monday is available on those days), the comment implies "Tomorrow" would be selected.

Consider aligning the comment with the actual implementation or using scheduleMessageForTomorrow() where the comment says "Tomorrow" is the option.

💡 Suggested comment fix

     /**
      * Picks the first available schedule option regardless of the current day.
      *
      * The picker shows different options depending on the day of the week:
      *   Sunday  (0): Tomorrow
      *   Monday  (1): Tomorrow, Next Monday
      *   Tue–Thu (2–4): Tomorrow, Monday
      *   Friday  (5): Monday
      *   Saturday(6): Monday
      *
-     * This method always selects a valid option so tests pass on any day.
+     * This method selects "Monday" (or "Next Monday" on Monday) for stability,
+     * since "Monday" is available on all days except Sunday where we also use it.
      */

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/support/ui/screen/channel.ts` around lines 420 - 446, The comment
describing day-of-week behavior for scheduleMessageForAvailableOption is out of
sync with the implementation: the code calls scheduleMessageForMonday() for
Sunday and Tue–Thu but the comment says "Tomorrow" is selected for those days;
either update the comment to state that Monday is chosen as a stable fallback on
Sunday and Tue–Thu, or change the logic to call scheduleMessageForTomorrow() for
the days where the comment expects "Tomorrow" (keep
scheduleMessageForNextMonday() for Monday and scheduleMessageForMonday() for
Fri/Sat). Ensure references to scheduleMessageForAvailableOption,
scheduleMessageForMonday, scheduleMessageForNextMonday, and
scheduleMessageForTomorrow are used so reviewers can locate the changes.

detox/e2e/support/ui/screen/channel_list.ts (1)

105-124: Category order may cause incorrect tap when channel appears in multiple categories.

If a channel appears in both 'unreads' and 'channels' (e.g., after receiving a new message), this method will always tap the 'channels' entry first since that's the order of iteration. This is likely acceptable for test scenarios but worth documenting.

The error message is clear about the failure mode but doesn't indicate which categories were searched, which could help debug flaky tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/support/ui/screen/channel_list.ts` around lines 105 - 124, The
tapSidebarPublicChannelDisplayName function may tap the wrong category due to
fixed iteration order and the thrown Error lacks context; update the error
thrown at the end of tapSidebarPublicChannelDisplayName to include the
categories that were searched (use the local categories constant and
channelName) so failures show which lists were probed (reference
tapSidebarPublicChannelDisplayName and getChannelItemDisplayName and the
categories array), e.g. build a message like "Sidebar channel item not found for
channel: X; searched categories: [..]" and throw that.

detox/e2e/test/products/channels/channels/archive_channel.e2e.ts (1)

221-255: Minor naming inconsistency: waitForSidebarPublicChannelDisplayNameVisible used for private channel.

At line 232, the test calls waitForSidebarPublicChannelDisplayNameVisible for a private channel. While functionally correct (the helper searches channels, unreads, favorites which includes private channels), the method name suggests it's for public channels only. This could cause confusion for future maintainers.

Consider renaming the helper to waitForSidebarChannelDisplayNameVisible (dropping "Public") in channel_list.ts to accurately reflect its capability, or add a separate helper for private channels.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/test/products/channels/channels/archive_channel.e2e.ts` around
lines 221 - 255, The test uses
ChannelListScreen.waitForSidebarPublicChannelDisplayNameVisible for a private
channel which is a naming mismatch; rename the helper in channel_list.ts from
waitForSidebarPublicChannelDisplayNameVisible to
waitForSidebarChannelDisplayNameVisible (preserve its implementation and
behavior) and update this test (and any other call sites) to call
ChannelListScreen.waitForSidebarChannelDisplayNameVisible(privateChannel.name,
timeouts.ONE_MIN) so the name no longer implies public-only channels.

detox/e2e/support/ui/screen/thread.ts (1)

225-229: Hardcoded testID string violates coding guidelines.

The 'scheduled_post_options_bottom_sheet' string should be defined as a testID constant rather than hardcoded in by.id(). As per coding guidelines: "Never hardcode strings in by.id() — always use page object testID constants."

♻️ Suggested fix

Add the testID constant to the class:

 testID = {
     threadScreenPrefix: 'thread.',
     threadScreen: 'thread.screen',
     backButton: 'screen.back.button',
     followButton: 'thread.follow_thread.button',
     followingButton: 'thread.following_thread.button',
     scheduledPostTooltipCloseButton: 'scheduled_post.tooltip.close.button',
     scheduledPostTooltipCloseButtonAdminAccount: 'scheduled_post_tutorial_tooltip.close',
+    scheduledPostOptionsBottomSheet: 'scheduled_post_options_bottom_sheet',
 };

Then update the reference:

             // Wait for the schedule picker bottom sheet using polling (no sync dependency).
             await waitForElementToExist(
-                element(by.id('scheduled_post_options_bottom_sheet')),
+                element(by.id(this.testID.scheduledPostOptionsBottomSheet)),
                 timeouts.HALF_MIN,
             );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@detox/e2e/support/ui/screen/thread.ts` around lines 225 - 229, Replace the
hardcoded 'scheduled_post_options_bottom_sheet' in the waitForElementToExist
call with a page-object testID constant: add a testID constant (e.g.,
SCHEDULED_POST_OPTIONS_BOTTOM_SHEET) to the Thread screen class testIDs (or
existing testIDs object) in thread.ts, then change
element(by.id('scheduled_post_options_bottom_sheet')) to
element(by.id(ThreadScreen.testIDs.SCHEDULED_POST_OPTIONS_BOTTOM_SHEET)) (or the
exact testIDs accessor used in this file). Ensure the constant name matches your
project's testID naming pattern and update any imports/usages accordingly; keep
the call to waitForElementToExist and function name waitForElementToExist
unchanged.